ENH: Display more information in MonteCarlo prints and plots #760

EvanMad · 2024-12-28T20:25:25Z

Pull request type

Code changes (bugfix, features)

Checklist

Lint (black rocketpy/ tests/) has passed locally
All tests (pytest tests -m slow --runslow) have passed locally
CHANGELOG.md has been updated (if relevant)

Current behavior

Prints maximum, minimum, mean, std dev
Plots ^ parameters with histograms

New behavior

Prints maximum, minimum, mean, std dev, 95% PI Lower, 95% PI Upper
Plots ^ parameters with histograms and boxplots

Breaking change

Yes
No

Additional information

For issue #730

Gui-FernandesBR · 2024-12-28T20:45:37Z

@EvanMad tks for your submission.

I've noticed this PR is duplicated with #753 , therefore we will have to think decide how to proceed, either merging both PRs into one or selecting just one of them.

Gui-FernandesBR

Overall it is looking good

Gui-FernandesBR · 2024-12-28T20:46:31Z

CHANGELOG.md

@@ -48,6 +48,7 @@ Attention: The newest changes should be on top -->

 - REL: update version to 1.7.1 in configuration files [#750](https://github.com/RocketPy-Team/RocketPy/pull/750)
 - MNT: Refactor Tank's testing Assertion with CAD data. [#678](https://github.com/RocketPy-Team/RocketPy/pull/678)
+ENH: Display more information in MonteCarlo prints and plots [#760](https://github.com/RocketPy-Team/RocketPy/pull/760)


Very minor, but new lines should be add to the top, not the bottom of sections. This is common confusion.

Ah I see, it's also in the wrong section. fixed now

Gui-FernandesBR · 2024-12-28T20:47:35Z

rocketpy/plots/monte_carlo_plots.py

        for key in keys:
-            plt.figure()
-            plt.hist(self.monte_carlo.results[key])
-            plt.title(f"Histogram of {key}")
-            plt.ylabel("Number of Occurrences")
+            fig, (ax1, ax2) = plt.subplots(2, 1, height_ratios=[1, 3], figsize=(8, 8))
+
+            # Plot boxplot
+            ax1.boxplot(self.monte_carlo.results[key], vert=False)
+            ax1.set_xlabel(key)
+            ax1.set_title(f"Box Plot of {key}")
+
+            # Plot histogram
+            ax2.hist(self.monte_carlo.results[key])
+            ax2.set_title(f"Histogram of {key}")
+            ax2.set_ylabel("Number of Occurrences")
+
+            plt.tight_layout()
            plt.show()


@Lucas-Prates should we really plot the 3 plot altogether? Perhaps letting the user activate/deactivate some of them if needed?

We could provide the option to activate/deactivate, but I see no harm in plotting these together. I mean, we get more information and the plot is not convoluted, looks fine to me. Moreover, I only see two plots (histogram + boxplot).

Gui-FernandesBR · 2024-12-28T20:50:56Z

rocketpy/simulation/monte_carlo.py

                mean = np.mean(values)
                stdev = np.std(values)
-                self.processed_results[result] = (mean, stdev)
+                pi_low = np.quantile(values, 0.025)
+                pi_high = np.quantile(values, 0.975)
+                median = np.median(values)
+                self.processed_results[result] = (mean, median, stdev, pi_low, pi_high)
            except TypeError:
-                self.processed_results[result] = (None, None)
+                self.processed_results[result] = (None, None, None, None, None)

    # Import methods


try: mean = np.mean(values) stdev = np.std(values) self.processed_results[result] = (mean, stdev) pi_low = np.quantile(values, 0.025) pi_high = np.quantile(values, 0.975) median = np.median(values) except TypeError: mean = None stdev = None pi_low = None pi_high = None median = None self.processed_results[result] = (mean, median, stdev, pi_low, pi_high)

Alternatively, we could make 5 try/except blocks instead of one single.

Oh yeah sorry i missed that, I have committed this suggestion

I don't think 5 try/except blocks is necessary. A situation where only one of these calculations fails seems unlikely to me. ie: either they all fail or none fail so catching them all in the same makes sense.

EvanMad · 2024-12-28T20:51:16Z

Apologies for duplicate. I saw that before opening the PR but it seemed like it was unfinished and contained errors

I believe this PR gets closer to resolving the original issue by combining the graphs onto the same plot

Let me know anything I can do to resolve this

Gui-FernandesBR · 2024-12-28T20:52:27Z

Apologies for duplicate. I saw that before opening the PR but it seemed like it was unfinished and contained errors

I believe this PR gets closer to resolving the original issue by combining the graphs onto the same plot

Let me know anything I can do to resolve this

No problems. Your approach looks good. Once again, thanks for your submission.

Gui-FernandesBR · 2024-12-29T15:56:43Z

@EvanMad our team is officially on vacations until January 6th.
We appreciate your patience when waiting for the final review.

Meanwhile, what you could already do is to solve linter issues pointed out by the GitHub Actions CI, or also solving the merge conflicts error.

Lucas-Prates

Thank you for your interest in solving this issue, @EvanMad. Overall, things look great to me! I was able to reproduce your plot when running the monte carlo usage notebook with the newest version (3.10) of matplotlib, which is good. Here are some remarks:

I encountered an issue when trying to call the plots.all method with version 3.5 of matplotlib, see image (1). This might be a problem since the current minimum requirement for matplotlib in our project is >=3.0 . If you wish to reproduce the error, install matplotlib==3.5 in a venv and run the monte carlo usage notebook;
This is merely for aesthetic reasons: we could remove the x-axis ticks, y-axis ticks, and x-axis label from the box plot, circled in red ellipsis in image (2); (unifying the plot titles might be a good idea as well, but nevermind this for now)

Image (1) - Error when plotting with version 3.5 of matplotlib

Image (2) - Suggested elements to be removed from the plot

Lucas-Prates · 2025-01-02T23:10:57Z

rocketpy/prints/monte_carlo_prints.py

+                print(
+                    f"{key:>25} {str(value[0]):>15} {str(value[1]):>15} {'N/A':>15} {'N/A':>15}"
+                )


Here we should make everything print "N/A". Moreover, it seems to be missing one more value so that they align correctly with the other rows, as shown in the last row for the image below.

Missed this, thanks, fixed it now :)

Thank you for this very useful review btw, I have fixed the aesthetic issues aswell as the linting CI failure.

I can see the issue with matplot lib, I am using the height_ratios property which was introduced in matplotlib 3.6, I haven't fixed this yet, will look into it in the next couple of days

I've fixed the issues on matplotlib==3.5! Let me know what else needs to be done to get this merged.

Do i need to worry about code coverage?

codecov · 2025-01-03T10:31:26Z

Codecov Report

Attention: Patch coverage is 72.72727% with 6 lines in your changes missing coverage. Please review.

Project coverage is 76.33%. Comparing base (83aa20e) to head (00148bc).
Report is 10 commits behind head on develop.

Files with missing lines	Patch %	Lines
rocketpy/simulation/monte_carlo.py	44.44%	5 Missing ⚠️
rocketpy/prints/monte_carlo_prints.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #760      +/-   ##
===========================================
- Coverage    76.42%   76.33%   -0.09%     
===========================================
  Files           95       96       +1     
  Lines        11090    11473     +383     
===========================================
+ Hits          8475     8758     +283     
- Misses        2615     2715     +100

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

done by removing height_ratios and replacing with gridspec

Evan Madurai added 3 commits December 27, 2024 17:02

init: new prints test

89bd6e1

move new prints into mc class itself

6c1c439

enh: boxplots added to histograms in montecarlo plots

34d3201

EvanMad marked this pull request as ready for review December 28, 2024 20:27

EvanMad requested a review from a team as a code owner December 28, 2024 20:27

DOC: update changelog

e5e9e09

Gui-FernandesBR requested a review from Lucas-Prates December 28, 2024 20:44

Gui-FernandesBR reviewed Dec 28, 2024

View reviewed changes

Evan Madurai added 2 commits December 28, 2024 21:03

DOC: fix changelog issues

b0137e1

ENH: cleaner calculation of result parameters

7a4a6b9

EvanMad changed the base branch from master to develop December 28, 2024 21:19

Lucas-Prates reviewed Jan 2, 2025

View reviewed changes

Evan Madurai and others added 4 commits January 3, 2025 00:34

enh: cleanup monte-carlo plots

084f32c

STY: linting fixes and re-format

01d0c28

STL: remove extra files

196c4f0

Merge branch 'develop' into enh/monte-carlo-plots

00148bc

BUG: fix compatability with matplotlib==3.5

3268fe8

done by removing height_ratios and replacing with gridspec

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Display more information in MonteCarlo prints and plots #760

ENH: Display more information in MonteCarlo prints and plots #760

EvanMad commented Dec 28, 2024 •

edited

Loading

Gui-FernandesBR commented Dec 28, 2024

Gui-FernandesBR left a comment

Gui-FernandesBR Dec 28, 2024

EvanMad Dec 28, 2024

Gui-FernandesBR Dec 28, 2024

Lucas-Prates Jan 2, 2025 •

edited

Loading

Gui-FernandesBR Dec 28, 2024

EvanMad Dec 28, 2024

EvanMad commented Dec 28, 2024

Gui-FernandesBR commented Dec 28, 2024

Gui-FernandesBR commented Dec 29, 2024

Lucas-Prates left a comment •

edited

Loading

Lucas-Prates Jan 2, 2025

EvanMad Jan 3, 2025 •

edited

Loading

EvanMad Jan 3, 2025

EvanMad Jan 5, 2025

codecov bot commented Jan 3, 2025

ENH: Display more information in MonteCarlo prints and plots #760

Are you sure you want to change the base?

ENH: Display more information in MonteCarlo prints and plots #760

Conversation

EvanMad commented Dec 28, 2024 • edited Loading

Pull request type

Checklist

Current behavior

New behavior

Breaking change

Additional information

Gui-FernandesBR commented Dec 28, 2024

Gui-FernandesBR left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lucas-Prates Jan 2, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvanMad commented Dec 28, 2024

Gui-FernandesBR commented Dec 28, 2024

Gui-FernandesBR commented Dec 29, 2024

Lucas-Prates left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EvanMad Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 3, 2025

Codecov Report

EvanMad commented Dec 28, 2024 •

edited

Loading

Lucas-Prates Jan 2, 2025 •

edited

Loading

Lucas-Prates left a comment •

edited

Loading

EvanMad Jan 3, 2025 •

edited

Loading